Ensure dashboard panels appear in the correct order#8540
Ensure dashboard panels appear in the correct order#8540stacey-gammon merged 1 commit intoelastic:masterfrom
Conversation
|
Jenkins, test this |
Bargs
left a comment
Bargs
left a comment
There was a problem hiding this comment.
Couple of minor inline comments, but functionality looks good!
There was a problem hiding this comment.
This should be expect(foo8Panel).to.not.be(null);
There was a problem hiding this comment.
seems to work (chai 3.5)? http://chaijs.com/api/bdd/#method_null
There was a problem hiding this comment.
We don't use Chai we use expect.js :)
There was a problem hiding this comment.
ah, where are we using chai then (we have dependency on it)?
There was a problem hiding this comment.
Looking at the Git blame, it looks like Rashid added that dependency when merging timelion, so I guess timelion uses chai.
There was a problem hiding this comment.
Good catch, indeed, expect(null).to.not.be.null; passes. Comments addressed.
tylersmalley
left a comment
tylersmalley
left a comment
There was a problem hiding this comment.
LGTM. This PR definitely fixes the issue, which many will rejoice about. It does open up an issue I see which is our use of the Gridster library, which mostly stems from it being a jQuery library, being abandonded and not consisting of any tests. At some point it would make sense to either roll our own or find Angular library to replace it with.
There was a problem hiding this comment.
To keep the project consistent, mind removing this blank line?
|
@stacey-gammon before merging, would you mind squashing everything into one commit? The commit history is a little tough to follow for the number of changes that actually exist in this PR :) |
stacey-gammon
force-pushed
the
fix-dashboard-panel-order
branch
from
2dc012d to
e531aef
Compare
Backports elastic#8540 Former-commit-id: 50d41a2
5 participants
Gridster apparently has an issue with rendering panels in the correct order. A work around is to pre-order the panels before passing them to gridster. See ducksboard/gridster.js#147 for reference.
Tests added that verify, at minimum, the reproducible scenario described by @Bargs in the issue #2138 is solved.
Note that this change may abruptly rearrange dashboards for some users. The reason is that the data is stored correctly, but displayed incorrectly. So for instance, someone created a dashboard, then attempted to swap vizA with vizB. They save the dashboard and the move failed to persist, so the user continues to see the original order. The data is still stored as if vizA was indeed swapped with vizB, which means with this change, the next time they load the data they will see their original intention manifested. Depending on how long ago they made that swap, this may come as a surprise. Probably something we will want to document.
Fixes #2138